stream: ensure pipeline always destroys streams #31940

ronag · 2020-02-24T22:41:46Z

There was an edge case where an incorrect assumption was made
in regards to whether eos/finished means that the stream is
actually destroyed or not.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

There was an edge case where an incorrect assumption was made in regardos whether eos/finished means that the stream is actually destroyed or not.

nodejs-github-bot · 2020-02-24T22:44:24Z

CI: https://ci.nodejs.org/job/node-test-pull-request/29328/

ronag · 2020-02-24T22:45:23Z

This is blocking backport of #31223 to node V13 which does not have autoDestroy enabled by default, thus some tests fail.

mcollina

lgtm

mcollina

If this is a backport it should target the relative branch (v13-staging in this case). Moreover I would expect to see a rather different change compared to what is on master.

ronag · 2020-02-25T09:03:17Z

If this is a backport it should target the relative branch (v13-staging in this case). Moreover I would expect to see a rather different change compared to what is on master.

This is not a backport. It fixes a problem on master for streams that do not autoDestroy. However, it should be backported as well and is required for backporting #31223 to v13. Since in master we changed default autoDestroy to true which is not (and will not be) included in v13.

mcollina

lgtm

nodejs-github-bot · 2020-02-25T09:43:53Z

CI: https://ci.nodejs.org/job/node-test-pull-request/29334/

There was an edge case where an incorrect assumption was made in regardos whether eos/finished means that the stream is actually destroyed or not. PR-URL: #31940 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

ronag · 2020-02-26T22:48:59Z

Landed in b2be348

There was an edge case where an incorrect assumption was made in regardos whether eos/finished means that the stream is actually destroyed or not. Backport-PR-URL: nodejs#31975 PR-URL: nodejs#31940 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

There was an edge case where an incorrect assumption was made in regardos whether eos/finished means that the stream is actually destroyed or not. Backport-PR-URL: #31975 PR-URL: #31940 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

arcanis · 2020-03-05T15:26:30Z

@ronag It seems this diff has introduced a major regression in Got:
sindresorhus/got#1107 (comment)

Note that I don't know if it's Got that relied on a buggy behavior or if it's an actual regression, but I figured I should ref the issues together 🙂

If the last stream in a pipeline is still usable/readable don't destroy it to allow further composition. Fixes: #32105 PR-URL: #32110 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

targos · 2020-04-20T19:40:00Z

Should this land on v12.x-staging? If so, please open a backport PR.

ronag · 2020-04-20T19:52:00Z

Should not land on 12.x. Has indirect consequences that can cause breakage.

mafintosh · 2020-04-20T20:05:05Z

Does this work with things like a TCP stream where destroy actually destroys the underlying fd was finish still can mean its waiting for FIN ack?

ronag · 2020-04-20T20:08:32Z

Does this work with things like a TCP stream where destroy actually destroys the underlying fd was finish still can mean its waiting for FIN ack?

Sorry, I didn't get that?

mafintosh · 2020-04-20T20:18:05Z

simple example pipeline(socket, socket). This calls destroy automatically after eos now yes? Ie basically forces autoDestroy if I’m understanding that correctly.

For streams that have postfinish state this can be problematic. For example TCP (this may have changed since I looked at it last), the stream emits finish when all pending data has been sent but end() has a side effect. Its sends a FIN packet also. Now you auto destroy it meaning FIN wont get resend if that packet is lost.

My utp-native (tcp like stream over udp) module will def break because of this due to o/

ronag · 2020-04-20T20:25:24Z

For streams that have postfinish state this can be problematic. For example TCP (this may have changed since I looked at it last), the stream emits finish when all pending data has been sent but end() has a side effect. Its sends a FIN packet also. Now you auto destroy it meaning FIN wont get resend if that packet is lost.

I'm not familiar with this so I can't really answer. Maybe @addaleax or @jasnell?

If more work needs to be done, e.g. send a FIN packet, I think that should be handled by _destroy alternatively 'finish' should be delayed until the FIN stuff is complete (I think this might be what currently happens?).

Though, in v14 pipeline won't call destroy as it assumes that "standard" streams (such as net.Socket since #31806) will handle it themselves (#32158).

mafintosh · 2020-04-20T20:28:00Z

follow up: wont this break all duplex streams?

net.createServer(function (socket) {
  pipeline(socket, a)
  pipeline(b, socket)
})

Wont this destroy the socket when either of those pipelines are done?

mafintosh · 2020-04-20T20:29:21Z

I can see tcp uses autoDestroy so it that part is prob fine, re my first q

ronag · 2020-04-20T20:30:22Z

Wont this destroy the socket when either of those pipelines are done?

There has been follow up PRs that ensure that if:

The first stream is writable, it won't be destroyed
The last stream is readable, it won't be destroyed

The current implementation is a little bit messy, but if you look at this PR which cleans it up a bit it might be clear.

ronag · 2020-04-20T20:32:40Z

@mafintosh Anything still left unanswered?

mafintosh · 2020-04-20T20:39:48Z

@ronag i think the other pr solves my above issue. Didn’t realise this was older.

mafintosh · 2020-04-20T20:41:58Z

Have you tested this with http servers? Calling destroy on a req destroys the full socket I think

ronag · 2020-04-20T20:43:28Z

Have you tested this with http servers? Calling destroy on a req destroys the full socket I think

Yes, I noticed

mafintosh · 2020-04-20T21:54:42Z

Heads up, this breaks tar-stream also due to the same reasons as HTTP. Just verified on latest 13.

mafintosh · 2020-04-20T21:59:19Z

Test case

echo hello > hello.txt
echo world > world.txt
tar c hello.txt world.txt > test.tar

const tar = require('tar-stream')
const fs = require('fs')
const path = require('path')
const pipeline = require('stream').pipeline

fs.createReadStream('test.tar')
  .pipe(tar.extract())
  .on('entry', function (header, stream, done) {
    console.log(header.name) // in 13 this will only unpack one file due to
                                                 // pipeline destroying the entire stream
    pipeline(stream, fs.createWriteStream(path.join('/tmp', header.name)), done)
  })

mafintosh · 2020-04-20T22:02:25Z

Let me put that into a proper issue

mafintosh · 2020-04-20T22:45:25Z

Thanks for the navigation help @ronag :)

This PR logically reverts nodejs#31940 which has caused lots of unnecessary breakage in the ecosystem. This PR also aligns better with the actual documented behavior: `stream.pipeline()` will call `stream.destroy(err)` on all streams except: * `Readable` streams which have emitted `'end'` or `'close'`. * `Writable` streams which have emitted `'finish'` or `'close'`. The behavior introduced in nodejs#31940 was much more aggressive in terms of destroying streams. This was good for avoiding potential resources leaks however breaks some common assumputions in legacy streams. Furthermore, it makes the code simpler and removes some hacks. Fixes: nodejs#32954 Fixes: nodejs#32955

This PR logically reverts #31940 which has caused lots of unnecessary breakage in the ecosystem. This PR also aligns better with the actual documented behavior: `stream.pipeline()` will call `stream.destroy(err)` on all streams except: * `Readable` streams which have emitted `'end'` or `'close'`. * `Writable` streams which have emitted `'finish'` or `'close'`. The behavior introduced in #31940 was much more aggressive in terms of destroying streams. This was good for avoiding potential resources leaks however breaks some common assumputions in legacy streams. Furthermore, it makes the code simpler and removes some hacks. Fixes: #32954 Fixes: #32955 PR-URL: #32968 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Mathias Buus <[email protected]>

This PR logically reverts nodejs#31940 which has caused lots of unnecessary breakage in the ecosystem. This PR also aligns better with the actual documented behavior: `stream.pipeline()` will call `stream.destroy(err)` on all streams except: * `Readable` streams which have emitted `'end'` or `'close'`. * `Writable` streams which have emitted `'finish'` or `'close'`. The behavior introduced in nodejs#31940 was much more aggressive in terms of destroying streams. This was good for avoiding potential resources leaks however breaks some common assumputions in legacy streams. Furthermore, it makes the code simpler and removes some hacks. Fixes: nodejs#32954 Fixes: nodejs#32955 PR-URL: nodejs#32968 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Mathias Buus <[email protected]> Backport-PR-URL: nodejs#32980

This PR logically reverts #31940 which has caused lots of unnecessary breakage in the ecosystem. This PR also aligns better with the actual documented behavior: `stream.pipeline()` will call `stream.destroy(err)` on all streams except: * `Readable` streams which have emitted `'end'` or `'close'`. * `Writable` streams which have emitted `'finish'` or `'close'`. The behavior introduced in #31940 was much more aggressive in terms of destroying streams. This was good for avoiding potential resources leaks however breaks some common assumputions in legacy streams. Furthermore, it makes the code simpler and removes some hacks. Fixes: #32954 Fixes: #32955 PR-URL: #32968 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Mathias Buus <[email protected]>

ronag added the stream Issues and PRs related to the stream subsystem. label Feb 24, 2020

ronag requested review from mcollina, mafintosh and lpinca February 24, 2020 22:41

stream: ensure pipeline always destroys streams

5a55b1d

There was an edge case where an incorrect assumption was made in regardos whether eos/finished means that the stream is actually destroyed or not.

ronag force-pushed the stream-pipeline-always-destroy branch from 23c5c02 to 5a55b1d Compare February 24, 2020 22:42

mcollina approved these changes Feb 25, 2020

View reviewed changes

mcollina requested changes Feb 25, 2020

View reviewed changes

mcollina approved these changes Feb 25, 2020

View reviewed changes

ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 25, 2020

BridgeAR approved these changes Feb 25, 2020

View reviewed changes

lpinca approved these changes Feb 25, 2020

View reviewed changes

ronag closed this Feb 26, 2020

codebytere mentioned this pull request Mar 1, 2020

v13.10.0 proposal #32027

Merged

szmarczak mentioned this pull request Mar 5, 2020

Pipeline aborts all HTTP requests #32105

Closed

targos added the backport-requested-v12.x label Apr 20, 2020

ronag added dont-land-on-v12.x and removed backport-requested-v12.x labels Apr 20, 2020

This was referenced Apr 20, 2020

tar-stream regression in 13 with pipeline #32954

Closed

Duplex stream pipeline regression in 13 #32955

Closed

ronag mentioned this pull request Apr 21, 2020

stream: pipeline should only destroy un-finished streams. #32968

Closed

4 tasks

ronag mentioned this pull request Apr 21, 2020

[v13.x backport] stream: pipeline should only destroy un-finished streams. #32980

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stream: ensure pipeline always destroys streams #31940

stream: ensure pipeline always destroys streams #31940

ronag commented Feb 24, 2020 •

edited

Loading

nodejs-github-bot commented Feb 24, 2020

ronag commented Feb 24, 2020

mcollina left a comment

mcollina left a comment

ronag commented Feb 25, 2020 •

edited

Loading

mcollina left a comment

nodejs-github-bot commented Feb 25, 2020

ronag commented Feb 26, 2020

arcanis commented Mar 5, 2020 •

edited

Loading

targos commented Apr 20, 2020

ronag commented Apr 20, 2020

mafintosh commented Apr 20, 2020

ronag commented Apr 20, 2020

mafintosh commented Apr 20, 2020

ronag commented Apr 20, 2020 •

edited

Loading

mafintosh commented Apr 20, 2020

mafintosh commented Apr 20, 2020

ronag commented Apr 20, 2020 •

edited

Loading

ronag commented Apr 20, 2020

mafintosh commented Apr 20, 2020

mafintosh commented Apr 20, 2020

ronag commented Apr 20, 2020 •

edited

Loading

mafintosh commented Apr 20, 2020 •

edited

Loading

mafintosh commented Apr 20, 2020

mafintosh commented Apr 20, 2020

mafintosh commented Apr 20, 2020

stream: ensure pipeline always destroys streams #31940

stream: ensure pipeline always destroys streams #31940

Conversation

ronag commented Feb 24, 2020 • edited Loading

Checklist

nodejs-github-bot commented Feb 24, 2020

ronag commented Feb 24, 2020

mcollina left a comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

ronag commented Feb 25, 2020 • edited Loading

mcollina left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Feb 25, 2020

ronag commented Feb 26, 2020

arcanis commented Mar 5, 2020 • edited Loading

targos commented Apr 20, 2020

ronag commented Apr 20, 2020

mafintosh commented Apr 20, 2020

ronag commented Apr 20, 2020

mafintosh commented Apr 20, 2020

ronag commented Apr 20, 2020 • edited Loading

mafintosh commented Apr 20, 2020

mafintosh commented Apr 20, 2020

ronag commented Apr 20, 2020 • edited Loading

ronag commented Apr 20, 2020

mafintosh commented Apr 20, 2020

mafintosh commented Apr 20, 2020

ronag commented Apr 20, 2020 • edited Loading

mafintosh commented Apr 20, 2020 • edited Loading

mafintosh commented Apr 20, 2020

mafintosh commented Apr 20, 2020

mafintosh commented Apr 20, 2020

ronag commented Feb 24, 2020 •

edited

Loading

ronag commented Feb 25, 2020 •

edited

Loading

arcanis commented Mar 5, 2020 •

edited

Loading

ronag commented Apr 20, 2020 •

edited

Loading

ronag commented Apr 20, 2020 •

edited

Loading

ronag commented Apr 20, 2020 •

edited

Loading

mafintosh commented Apr 20, 2020 •

edited

Loading